-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Distribute ES module #875
base: master
Are you sure you want to change the base?
Distribute ES module #875
Conversation
Hi Jordan, Thanks for your work on the topic, this is a step forward on the library. Did you have some remarks for me to take in consideration before testing it? Should we update something of the documentation/readme? |
@Silic0nS0ldier Test are failing for Node10 Could you please check it? |
Its bark is worse then its bite here. API surface remains unchanged, most of the noise comes from the removal of an indent layer across the bulk of the main source file and GitHub struggling to understand that a rename has occurred (usually happens when generated files are tracked). Using a separate diff tool and temporarily adding back the indent layer should help to put focus in the right places. Definitely needs a major release here. The worker logic change represents a breaking change (changed requirements) and the addition of the es module will almost certainly break someone's build (from experience). Any documentation changes would be to cover changes worker requirements and including the es module in docs as appropriate (there are a few links to the umd and mini umd scripts). |
CI test failure comes down to Mocha v9 dropping support for NodeJS 10. Dropping down to v8 should address that. |
@Silic0nS0ldier I see node10 is no longer supported so I removed support for it in #876 Note that we are no longer using the Travis CI infrastructure so I removed the file to avoid confusions. Could you please rebase the PR to latest master branch? This will only run the tests with supported node versions, so you should be able to bump the mocha version back to 9.0 Sorry for the confusions. |
I did an integration test with an existing project. Only issue was a minor tooling break due to |
@Silic0nS0ldier Good news. Could you ping me back when you've finished the integration project so I can come back to test it here? Also I'm wondering why you used RollupJS? Correct me if I'm wrong but is not also possible to use webpack? Which are the benefits of using RollupJS? |
Project in question is closed source sorry.
I mostly opted for RollupJS as its what I have experience using. Beyond that though, the consensus I've seen is that webpack is best suited to apps while RollupJS is best suited to libraries (no real justification for why that is though). Key difference I see is that historically RollupJS produced a more optimised output, I understand that as of webpack v2 the difference became much less significant. As for benefits;
There are quite a few other alternatives out there now (most targeting webpack). Some of them are;
|
I would say drop support for commonjs entierly and do a major release (there are other popular modules that have already done so) |
@jimmywarting Current versions of PapaParse are distributed as a UMD (specifically the pattern https://github.com/umdjs/umd/blob/master/templates/returnExports.js) which supports binding to I'll await for a consensus to form before making any changes on this front. |
I do not think we should drop support fort commonjs. |
That's true, You will not benefit so much of it right now if you can output both UMD and ESM.
node-fetch, fetch-blob, formdata-polyfill are 3 that i know of that only support ESM and have ditched commonjs, or any UMD pattern |
Regarding shipping ESM only, there is one major consideration. Should a project be consuming the CJS and ESM versions of the source (this is possible, particularly if PapaParse is used in another library and directly), global configuration won't be shared. While it is possible to address this by having global configuration stored in a common CJS file, doing so is likely to change the API surface. Shipping ESM only can however present challenges on the consumer side where ESM is not fully adopted. Tools like webpack are quite good at hiding missing ESM support and introducing behaviour which does not exist in native ESM. This is a problem for a monolithic project I work on which uses PapaParse in the following contexts;
Moving to proper ESMis something being worked towards, however there are a lot of hurdles to overcome. As such I'm in favour of keeping a CJS export around for at least the next release. |
any update on ESM release? |
Not sure if I'm the holdup here, but I won't have time to make any changes to this PR. If others are waiting on this, feel free to open another PR (link to it, and I'll close this PR) |
Intent in this PR is to convert the existing source into a valid ES module which can be distributed. More could be done, but IMO would be too much all at once.
Maintaining backwards compatibility where possible was a goal with these changes, however there is a limit to how far this can go. Even if the existing files (
papaparse.js
andpapaparse.min.js
) were not modified, the presence of the ES module (andexports
inpackage.json
) are highly likely to impact downstream projects in the form of build breaks and runtime errors. These are issues which are easier to overcome when not compounded by other significant changes, which is where backward compatibility efforts were spent.For consumers, changes of note are;
require.resolve
orimport.meta.resolve
that is not defined in the export map)import.meta.url
, see https://caniuse.com/?search=import.metadocument.currentScript.src
, see https://caniuse.com/?search=document.currentScriptFor maintainers, changes of note are;
papaparse.mjs
, formerlypapaparse.js
papaparse.js
andpapaparse.min.js
.^4.19.1
to^7.28.0
2020
(minimum forimport.meta
support)import.meta.url
for ES modules anddocument.currentScript.src
for UMD in browser, and identify themselves as PapaParse created workers via the worker scope name (formerly used presence ofblob
in script URL).mocha-headless-chrome
updated.Primary driver was to address crashes in an old version of puppeteer.
One change of note is that Mocha (now at
^9.0.0
) supports loading ES module test files (not currently used).Related
#748
#813